Improve contributing tutorial documentation#486
Conversation
- Fix Dataset API example to use DatasetSchema instead of plain dict - Add pre-flight environment checklist to Prerequisites section - Add note about PAT workflow scope requirement in Step 7 Closes Reed-CompBio#480
ntalluri
left a comment
ntalluri
left a comment
There was a problem hiding this comment.
This looks good to me. I left some questions on stuff that could be added or changed but they're more suggestions.
| docker login | ||
| conda --version | ||
| git --version | ||
|
|
There was a problem hiding this comment.
should we also add a check to make sure SPRAS conda env is ready?
There was a problem hiding this comment.
python -c "import spras; print('SPRAS import successful')" is what is in the spras tutorial.
|
|
||
| .. note:: | ||
|
|
||
| If you are using HTTPS to push and your commit touches files in |
There was a problem hiding this comment.
Can we just explicitly say for developers to use SSH to push to SPRAS in the contributing guide? And then add a warning that if you use https the issues you mentioned will happen.
There was a problem hiding this comment.
If this affects the workflows updates, let's put it in the step that first affects files there. That's Step 5, right?
There are two types of tokens, which makes things even more confusing. How is this rephrased language?
These tests modify files in
.github/workflows/. If you push over HTTPS using a Personal Access Token, make sure the token has permission to modify GitHub Actions workflows. Otherwise, GitHub may reject the push. SSH push works without this restriction.
| ``main`` branch of the fork stays synchronized with the ``main`` branch | ||
| of the main SPRAS repository. | ||
|
|
||
|
|
There was a problem hiding this comment.
Can we remove these new white space lines?
agitter
left a comment
agitter
left a comment
There was a problem hiding this comment.
If we want to add another Note section in the Prereqs with some of the suggestions for macOS users, that could be a place to provide links about the different shell, miniconda, OrbStack, etc.
I don't see a better example Dockerfile than PathLinker because I find the COPY example important. If you want to propose a language change that suggests parts of the PathLinker Dockerfile to ignore, we could do that.
|
|
||
|
|
||
|
|
||
| Before getting started, verify your environment is ready with these checks: |
|
|
||
| .. code:: bash | ||
|
|
||
| docker --version |
There was a problem hiding this comment.
If we want to have these checks, should we also add the expected output? The specific output will be different depending on what each contributor has installed, but I think we should give some guidance about how someone can tell if everything is ready or not. Maybe the signal is that they don't get errors?
|
|
||
| .. note:: | ||
|
|
||
| If you are using HTTPS to push and your commit touches files in |
There was a problem hiding this comment.
If this affects the workflows updates, let's put it in the step that first affects files there. That's Step 5, right?
There are two types of tokens, which makes things even more confusing. How is this rephrased language?
These tests modify files in
.github/workflows/. If you push over HTTPS using a Personal Access Token, make sure the token has permission to modify GitHub Actions workflows. Otherwise, GitHub may reject the push. SSH push works without this restriction.
3 participants
Summary
Addresses documentation gaps identified in #480.
Changes made
DatasetAPI example in Step 3 to useDatasetSchemainstead of a plain dict (the old example fails with the current API)workflowscope requirement in Step 7Out of scope for this PR (team decisions needed)
cc @agitter
cc @ntalluri